Skip to content

Conversation

@deshipu
Copy link

@deshipu deshipu commented Aug 3, 2019

This also allows us to control the size of the buffer. Half of the
buffer will be used for the fist, and half for the second internal
buffer.

It lets us re-use the same buffer for playing multiple files.
This also allows us to control the size of the buffer. Half of the
buffer will be used for the fist, and half for the second internal
buffer.
@dhalbert
Copy link
Collaborator

dhalbert commented Aug 3, 2019

Good idea! A few thoughts pre-review:

  • The doc should probably say that if a buffer is not passed in, a buffer of 512 bytes (or say that two buffers of 256 bytes) will be used. This will give the user guidance about how big to make the buffer.
  • Should one buffer that's split be passed, or two? If two, the smaller of the two will determine the length used. (Or they could be required to be of equal length). The advantage of two buffers is that they might be easier to allocate in the face of fragmentation. The internal implementation allocates two buffers separately. On the other hand using a larger buffer that later gets freed would reduce fragmentation later.

@deshipu
Copy link
Author

deshipu commented Aug 3, 2019

Good point about documenting the default size, I will add that.

I went for a single buffer mostly because it simplifies the usage, and lets us skip explaining what double buffering is. I'm not worried about memory fragmentation, because it's going to be allocated at the beginning of your program anyways.

By the way, experiments show that a much smaller buffer than 512 seems to be working just fine, so we might want to reconsider the default.

@deshipu
Copy link
Author

deshipu commented Aug 3, 2019

I suppose the optimal buffer size depends on the bit rate.

@dhalbert
Copy link
Collaborator

dhalbert commented Aug 3, 2019

I suppose the optimal buffer size depends on the bit rate.

And speed of processor, and what other background tasks are happening, so it's probably hard to specify without experimentation.

@deshipu
Copy link
Author

deshipu commented Aug 3, 2019

All the better to have it user-controlled.

jepler
jepler previously approved these changes Aug 10, 2019
Copy link

@jepler jepler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine, I didn't spot any problems.

As we discussed on discord, I hope that in the future we can revisit this and reduce runtime memory allocations further (and maybe reduce memory allocated overall) by putting the AudioOut object in the driver's seat. But that work's not started, and this presents a solid improvement right now.

@deshipu deshipu requested a review from tannewt August 16, 2019 14:38
@tannewt
Copy link
Member

tannewt commented Aug 16, 2019

@dhalbert Please merge if you don't want to review.

@tannewt tannewt removed their request for review August 16, 2019 18:55
@deshipu
Copy link
Author

deshipu commented Aug 16, 2019

I actually just realized that I didn't add the information about default buffer size to the documentation. I will do that tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants